-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Avoid infinite redirection #4833
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4833 +/- ##
=========================================
+ Coverage 76.2% 76.2% +<.01%
=========================================
Files 158 158
Lines 10022 10024 +2
Branches 1265 1266 +1
=========================================
+ Hits 7637 7639 +2
Misses 2041 2041
Partials 344 344
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a good change. Had some thoughts on the UX around it, but don't have an obvious way forward with it.
readthedocs/core/views/__init__.py
Outdated
@@ -116,7 +116,9 @@ def server_error_404(request, exception=None, template_name='404.html'): # pyli | |||
Marking exception as optional to make /404/ testing page to work. | |||
""" | |||
response = get_redirect_response(request, path=request.get_full_path()) | |||
if response: | |||
|
|||
if response and response.url != request.build_absolute_uri(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should add some logging here if there is a loop. It would be best to actually be able to alert the user, because presumably they have misconfigured a redirect and will be confused. Not sure the best course of action tho, because doing a debug
log will never be shown in prod :/
Perhaps we should show a different error code in this case, perhaps a 500 level, to show that something is broken vs. just missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should add some logging here if there is a loop
👍 on adding a log.warning
if this happens.
Perhaps we should show a different error code in this case, perhaps a 500 level, to show that something is broken vs. just missing?
Mmm... Well, for the use case were we detected this, what we want here is a 404 not a 500 actually (you can read the issue that this PR closes for more context).
I suppose that returning a 404 is the first step here to avoid this problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, better than actually returning an infinite redirect. So I'm +1 on shipping and then improving later.
Closes #4673